Skip to content

Fix function definition regex bug #6547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 21, 2022
Merged

Fix function definition regex bug #6547

merged 2 commits into from
Jan 21, 2022
+303 −57

Conversation

@gregersrygg
Copy link
Contributor

@gregersrygg gregersrygg commented Jan 14, 2022 •
edited
Loading

The whitespace fix to this regex last week triggered another bug in
libmodem. They have a typedef that the regex parsed as a function
definition because it accepted too many character in the argument
list of a function. This fix updates the regex to only allow
characters you would expect in an argument list of a function def.

Characters expected in the argument list of a function def:

\w A-Z a-z 0-9 and _
\s All whitespace
,  Argument separator
*  Pointers
\. Variadic argument
\[ Array start
\] Array end

@gregersrygg gregersrygg self-assigned this Jan 14, 2022
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 14, 2022
@gregersrygg
scripts: unity: Fix function definition regex bug
d3fdbd0 
The whitespace fix to this regex last week triggered another bug in
libmodem. They have a typedef that the regex parsed as a function
definition because it accepted too many character in the argument
list of a function. This fix updates the regex to only allow
characters you would expect in an argument list of a function def.

Characters expected in the argument list of a function def:
\w A-Z a-z 0-9 and _
\s All whitespace
,  Argument separator
*  Pointers
\. Variadic argument
\[ Array start
\] Array end

Signed-off-by: Gregers Gram Rygg <[email protected]>
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jan 14, 2022

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

lemrey
lemrey approved these changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@lemrey lemrey left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gregersrygg, mocks are generated correctly in libmodem now.
I wonder if the CI on this PR runs any unit tests though, we should make sure it does.

@lemrey lemrey removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 14, 2022
@lemrey
Copy link
Contributor

lemrey commented Jan 17, 2022

@nordic-krch , @tejlmand please have a look.

tejlmand
tejlmand requested changes Jan 18, 2022
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a test for your new regex here with an example of what you're fixing:
https://github.com/nrfconnect/sdk-nrf/blob/main/tests/unity/example_test/src/foo/foo.h

feel free to add such tests in other files under https://github.com/nrfconnect/sdk-nrf/blob/main/tests/unity/example_test if more appropriate.

Please also add a test for the previous fix you made here: #6478

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 19, 2022
@gregersrygg
Copy link
Contributor Author

gregersrygg commented Jan 19, 2022

@tejlmand Yes, I noticed the example_test while going through previous commits on some of the files I touched. I also felt the need for more testing on this functionality. However I didn't want to clutter the example_test with more tests, so I separated the wrap tests from the example_test.

@tejlmand
Copy link
Contributor

tejlmand commented Jan 19, 2022

However I didn't want to clutter the example_test with more tests, so I separated the wrap tests from the example_test.

Perfectly fine.

Just wanted to ensure to get some test on this, so thanks for improving testing.

balaji-srin
balaji-srin reviewed Jan 19, 2022
View reviewed changes
balaji-srin
balaji-srin suggested changes Jan 19, 2022
View reviewed changes
@balaji-srin
Copy link
Contributor

balaji-srin commented Jan 19, 2022

I think it is better to write a README or some documentation explaining what example_test can be used for.

tejlmand
tejlmand reviewed Jan 19, 2022
View reviewed changes
tejlmand
tejlmand reviewed Jan 19, 2022
View reviewed changes
@gregersrygg
Copy link
Contributor Author

gregersrygg commented Jan 19, 2022

I also made a small experiment on using pycparser to parse the C code instead of using regexes. However it relies on the C preprocessor to run first (for #includes), so the cmake script would have to pass include paths to the python script, and it would also be difficult to output the modified C files with the #includes reverted back to how it was before the preprocessor. So it's not as easy as I had hoped.

@gregersrygg gregersrygg force-pushed the fix-func-def-regex branch 2 times, most recently from 8488cec to 806486a Compare January 19, 2022 10:44
balaji-srin
balaji-srin reviewed Jan 19, 2022
View reviewed changes
tejlmand
tejlmand reviewed Jan 19, 2022
View reviewed changes
@gregersrygg
Copy link
Contributor Author

gregersrygg commented Jan 19, 2022

@balaji-nordic
I think it is better to write a README or some documentation explaining what example_test can be used for.

This PR only removes code from the example_test, so I don't understand why I would then have to write documentation for for it as part of this bugfix/cleanup. There's also end user documentation for the example_test on our documentation site.

tejlmand
tejlmand reviewed Jan 19, 2022
View reviewed changes
@gregersrygg gregersrygg changed the title scripts: unity: Fix function definition regex bug Fix function definition regex bug Jan 20, 2022
tejlmand
tejlmand approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

Approved under the expectation that snippets.c/h will be renamed according to discussions.

@gregersrygg
tests: unity: Separate example test and wrap tests
12fc8ed 
The python scripts to prefix functions with __wrap was tested using the
example_test. This test is mean as an example of how to do unit
testing. Moved the existing wrap tests and some new ones to a new test
suite to avoid adding more irrelevant tests to the example test.

Signed-off-by: Gregers Gram Rygg <[email protected]>
@gregersrygg
Copy link
Contributor Author

gregersrygg commented Jan 21, 2022

The compliance check that fail is because of intentional syntax in tests/unity/wrap_test/src/test_code/test_code.h. It is formatted this way to mimic syntax that has broken the code parsing in the past:

@tejlmand
Copy link
Contributor

tejlmand commented Jan 21, 2022

@carlescufi or @mbolivar-nordic this is good to merge.

Compliance failures are expected, as test code need to break formatting to test that the scripts works also on 3rd party code that are not following our coding guideline (and hence our test headers replicating that formatting break our guidelines)

@mbolivar-nordic mbolivar-nordic merged commit 2a923d2 into nrfconnect:main Jan 21, 2022
@gregersrygg gregersrygg deleted the fix-func-def-regex branch January 24, 2022 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lemrey lemrey lemrey approved these changes

@tejlmand tejlmand tejlmand approved these changes

@mbolivar-nordic mbolivar-nordic Awaiting requested review from mbolivar-nordic

@nordic-krch nordic-krch Awaiting requested review from nordic-krch nordic-krch is a code owner

+1 more reviewer

@balaji-srin balaji-srin balaji-srin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gregersrygg gregersrygg

Labels

changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gregersrygg @NordicBuilder @lemrey @tejlmand @balaji-srin @mbolivar-nordic